-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a false positive for Style/CaseLikeIf
when conditional contains comparison with a class
#8511
Fix a false positive for Style/CaseLikeIf
when conditional contains comparison with a class
#8511
Conversation
def class_reference?(node) | ||
return false unless node.const_type? | ||
|
||
name = node.children[1].to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using node.children[1].match? /[[:lower:]]/
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code checks that const name is not all uppercase, so the code could be written as !name.match?(/^[[:upper:]]+$/)
, but this does not handle numbers in names and so, the original code looks simpler to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checks that const name is not all uppercase
Right. I'm sorry though, I don't see how "not all uppercase" != "has a lowercase", could you provide an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry. I mistakenly matched that regexp only with the first letter 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix. Could you also mark it as Safe: false
as there are many potential cases we're not identifying properly (any object where ===
and ==
is not the same, like Set for example...)
8ebec06
to
f3eb0ee
Compare
Marked it as unsafe. |
f3eb0ee
to
598f812
Compare
I'll merge after the merge conflict is resolved... 😢 |
… comparison with a class
598f812
to
8ae6e89
Compare
Thanks! |
Closes #8508